-
Notifications
You must be signed in to change notification settings - Fork 88
Feature/lit 4238 clean unnecessary packages #802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/lit 4238 clean unnecessary packages #802
Conversation
… moving and code refactors
| ## Prerequisite | ||
|
|
||
| - node (v19.x or above) | ||
| - node (v20.x or above) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v19 has gone out of maintenance a while ago
v20 is in maintenance and supports File (used in encryption)
| }); | ||
|
|
||
| log('[getPkpAuthContext]: ', authContext); | ||
| console.log('[getPkpAuthContext]: ', authContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing logs are more clear when using console than using pino
| "jest": "^29.2.2", | ||
| "jest-environment-jsdom": "^29.7.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ts-jest was complaining about jest old version and after v28 jest-environment-jsdom does not come automatically by default
| localStorage.removeItem(storage.AUTH_SOL_SIGNATURE); | ||
| localStorage.removeItem(storage.AUTH_COSMOS_SIGNATURE); | ||
| localStorage.removeItem(storage.WEB3_PROVIDER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these were used. We only had writes for some of them but no reads anywhere
| * | ||
| * @returns {boolean} - True if provider is supported | ||
| */ | ||
| export function isSocialLoginSupported(provider: string): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used anywhere and with the new architecture does not make a lot of sense. It is supported if there is a provider after all
| "updateBuildableProjectDepsInPackageJson": true | ||
| } | ||
| }, | ||
| "copyJSONFilesToDist": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used
| const MUST_HAVE_EVM_CHAINS: Array<string> = [ | ||
| 'ethereum', | ||
| 'polygon', | ||
| 'fantom', | ||
| 'xdai', | ||
| 'bsc', | ||
| 'arbitrum', | ||
| 'avalanche', | ||
| 'fuji', | ||
| 'harmony', | ||
| 'kovan', | ||
| 'mumbai', | ||
| 'goerli', | ||
| 'ropsten', | ||
| 'rinkeby', | ||
| 'cronos', | ||
| 'optimism', | ||
| 'celo', | ||
| 'aurora', | ||
| 'eluvio', | ||
| 'alfajores', | ||
| 'xdc', | ||
| 'evmos', | ||
| 'evmosTestnet', | ||
| 'bscTestnet', | ||
| 'baseGoerli', | ||
| ]; | ||
|
|
||
| const MUST_HAVE_SOL_CHAINS = ['solana', 'solanaDevnet', 'solanaTestnet']; | ||
| const MUST_HAVE_COSMOS_CHAINS = [ | ||
| 'cosmos', | ||
| 'kyve', | ||
| 'evmosCosmos', | ||
| 'evmosCosmosTestnet', | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Types enforce this now
|
nit: should we use standardise using |
I think we should. It has been supported since 2021 (node v16 and chrome 91) so I don't see any reason why not use it. However I would keep it in a followup to avoid making this bigger Opinions @MaximusHaximus ? |
|
Just a reminder that tslib / typescript at multiple had issues with the # markup, so we had intentionally avoided using it in #526 -- are we sure it's safe at this point? :D |
A rapid replace and build succeeded As per gpt But we are using tslib 2.7 and 2.3. We can let consumers know that tslib requires v2.0.0 (which was released 5 years ago) |
MaximusHaximus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that is missing w/ the new logger changes is the ability to configure the SDK logger, which is a pretty fundamental function for the logger. Since we create instances of pino in multiple places in module scope, and don't expose those instances, they are all using defaults for transports, formatting, log levels, etc.
I think we still need a (much simpler than before!) SDK-wide logger package, where sdk consumers can do things like set log level and transports for logs being generated by the SDK.
For example, even though we (and our SDK consumers), might want to see trace level logs sometimes, for every-day purposes many (most) consumers will not want to see that degree of granularity in the output. Right now, since the loggers are not exposed in any way, there is no way for their level to be set.
This also has further repercussions -- one of the big benefits of using pino for logging is that its transports, serializers, and formatters can be configured for various purposes -- e.g. to ship logs to an aggregation service, pretty-print them, or output them as structured JSON. Right now there is no way for consumers to configure those transports.
The simplest way to handle this is, rather than creating new pino instances directly inside of module scopes, we want the logger package to expose two basic methods:
(1) setLoggerOptions(), which can mutate the options that are provided to an sdk-wide singleton pino logger instance.
(2)getLogger(), which returns the singleton instance of our SDK pino logger. Anyt time getLogger() is called, the logger singleton instance will be constructed using the options that are set at that time if it does not already exist, otherwise the existing instance will be returned.
The idea is that, at the top of my app that uses the LIT SDK, before I call any SDK methods, I can configure the SDK logger completely using any supported pino options:
import { setLoggerOptions } from '@lit-protocol/logger';
setLoggerOptions({level: 10, transports: [...] }); // Here's where I would set up e.g. pino-pretty or other log formatting/destination options.
We can also allow mutation of logging configuration even after it has been used. Any time setLoggerOptions() is run, we could reset any existing pino singleton, so that the next time code runs that calls getLogger(), a new one is created with the newly set options.
With this approach, when a consumer of the SDK wants to control the log verbosity, transports, serializers, etc, they can call our method setLoggerOptions() to mutate the options that we use when constructing the logger singleton, and then use the SDK as-normal.
The critical change in structure inside our code is that everywhere we are currently constructing a logger in module scope, then using it directly inside of our function calls, we must modify it to call getLogger() and use the instance returned by that call to call [logger.info](http://logger.info/), logger.error, etc. — on the sdk-wide logger instance.
If we want to provide helpers for consumers of the SDK, we could provide some examples of options combinations for e.g. pretty-printing logs, outputting them to files, or exporting them to an external log sink — all of these would be references to the existing Pino transport and options APIs!
…te and underscore syntax to avoid issues with tslib until we decide they are safe for consumers
Great feedback. Considering that I readded the logger pkg to be the centralization piece of loggers. I tested this locally with a script I have and turned out to be a great config place, for example to use pino-pretty when testing locally |
…ons are extensible on machine construction and referenceable in machine definition
…is updated after import
…ecessary-packages
|
Merging this to avoid further divergence from the base PRs. Let’s create follow-up PRs if anything needs to be addressed. |


Description
This PR makes the following changes
miscpackage, mostly withzodschemas and function movingloggerpackage usingpinologgersuin8arraypackage. Replaced byBuffernaclpackage. Replaced with noble curvesencryptionpackage (just a wrapper over lit node clientencryptanddecryptmethods)localstorageaccess behindmisc-browser(to-be-replaced withstorageparam where needed)Note:
misc-browseris almost completely removed. It is merely a stricter wrapper oflocalstoragenow. However, as several places rely onlocalstoragefor caching it cannot be blindly removed. It has to be replaced with the storage options the base branch will implementNew package graph

Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: